[Heartbeat] Fix broken invocation of synth package #26228

andrewvc · 2021-06-10T00:47:26Z

Fixes invocation of synthetics broken in #26188

Additionally, for dev purposes, this lets accepts a synthetics version of file:/// as valid to help with debugging development versions of the synthetics agent.

Never released, so no changelog needed

Still just sticking with unit tests here since testing more deeply with synthetics (esp. master where this is a problem) is a larger problem than we're equipped to handle ATM.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~- [ ] I have made corresponding changes to the documentation~~
~~- [ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
~~- [ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.~~

How to test this PR locally

Can be done with file:// deps in package.json used to run the latest synthetics master branch

elasticmachine · 2021-06-10T01:38:40Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #26228 updated
Start Time: 2021-06-10T21:13:18.844+0000
Duration: 45 min 47 sec
Commit: 68af7d8

Test stats 🧪

Test	Results
Failed	0
Passed	86
Skipped	0
Total	86

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	86
Skipped	0
Total	86

vigneshshanmugam · 2021-06-10T05:01:39Z

x-pack/heartbeat/monitors/browser/synthexec/synthexec.go

@@ -30,7 +30,7 @@ const debugSelector = "synthexec"
 func SuiteJob(ctx context.Context, suitePath string, params common.MapStr, extraArgs ...string) (jobs.Job, error) {
 	// Run the command in the given suitePath, use '.' as the first arg since the command runs
 	// in the correct dir
-	newCmd, err := suiteCommandFactory(suitePath, append(extraArgs, ".")...)
+	newCmd, err := suiteCommandFactory(suitePath, append([]string{suitePath}, extraArgs...)...)


is this a bug?

It's intentional and fixes the bug.

Sorry, will add tests soon, I shouldn't have flagged you for review this early. The bug was we'd invoke with --capabilities foo bar . where . indicated the current directory. Synthetics interpreted . as a capability, not the location of the test. So, we needed to ensure that the suite path was the first arg. While I was at it I switched from . to suitePath which was more readable and makes copy/pasting the command easier.

Totally, no worries. Yes this is the issue with varidiac args in the commander library we are using - This is one of the reason why I was suggesting to add a test here - #25808 (comment)

Glad we caught the error. If you are curios - https://github.com/tj/commander.js/blob/master/docs/options-taking-varying-arguments.md

vigneshshanmugam

LGTM

vigneshshanmugam · 2021-06-10T22:01:09Z

x-pack/heartbeat/monitors/browser/source/validatepackage.go

@@ -42,6 +42,10 @@ func parseVersion(version string) string {
 }

 func validateVersion(expected string, current string) error {
+	if strings.HasPrefix(current, "file://") {


do we want to move this behind a flag? Just a thought though, what if users runs a custom locally modified synth version?

But I am also thinking, if they are using the flag, then we are in the same boat. your call.

elasticmachine · 2021-06-10T22:24:56Z

Pinging @elastic/uptime (Team:Uptime)

andrewvc · 2021-06-10T22:35:53Z

@Mergifyio backport 7.x

Fixes invocation of synthetics broken in #26188 Additionally, for dev purposes, this lets accepts a synthetics version of file:/// as valid to help with debugging development versions of the synthetics agent. Never released, so no changelog needed Still just sticking with unit tests here since testing more deeply with synthetics (esp. master where this is a problem) is a larger problem than we're equipped to handle ATM. (cherry picked from commit ad7c19f) # Conflicts: # x-pack/heartbeat/monitors/browser/source/validatepackage.go # x-pack/heartbeat/monitors/browser/source/validatepackage_test.go

mergify · 2021-06-10T22:38:09Z

Command backport 7.x: success

Backports have been created

#26250 [Heartbeat] Fix broken invocation of synth package (backport #26228) has been created for branch 7.x

* master: (26 commits) Report total and free CPU for vSphere virtual machines (elastic#26167) [filebeat] Add preserve_original_event option to o365audit input (elastic#26273) Change xml processor names in script processor to match convention (elastic#26263) [Oracle] Fixing default values for paths in config template (elastic#26276) Add more ECS fields to logs (elastic#25998) [Heartbeat] Fix broken invocation of synth package (elastic#26228) rename sqs file name (elastic#26227) Populate the agent action result if there is no matching action handlers (elastic#26152) Add ISO8601 as supported timestamp type (elastic#25564) Move Filebeat azure module to GA (elastic#26168) Filebeat azure module pipeline fixes and changes (elastic#26148) libbeat: monitor version (elastic#26214) Add new parser to filestream input: container (elastic#26115) [Metricbeat] Add state_statefulset replicas.ready (elastic#26088) Disable test processors system test for windows 10 (elastic#26216) Fix startup with failing configuration (elastic#26126) Remove 32 bits version of Elastic Agent. (elastic#25708) Chane fleetmode detection to ony use management.enabled (elastic#26180) Make `filestream` input GA (elastic#26127) libbeat/idxmgmt/ilm: fix alias creation (elastic#26146) ...

…26250) * [Heartbeat] Fix broken invocation of synth package (#26228) Fixes invocation of synthetics broken in #26188 Additionally, for dev purposes, this lets accepts a synthetics version of file:/// as valid to help with debugging development versions of the synthetics agent. Never released, so no changelog needed Still just sticking with unit tests here since testing more deeply with synthetics (esp. master where this is a problem) is a larger problem than we're equipped to handle ATM. (cherry picked from commit ad7c19f) # Conflicts: # x-pack/heartbeat/monitors/browser/source/validatepackage.go # x-pack/heartbeat/monitors/browser/source/validatepackage_test.go * Fix license * Fix notice Co-authored-by: Andrew Cholakian <andrew@andrewvc.com>

…26250) * [Heartbeat] Fix broken invocation of synth package (#26228) Fixes invocation of synthetics broken in #26188 Additionally, for dev purposes, this lets accepts a synthetics version of file:/// as valid to help with debugging development versions of the synthetics agent. Never released, so no changelog needed Still just sticking with unit tests here since testing more deeply with synthetics (esp. master where this is a problem) is a larger problem than we're equipped to handle ATM. (cherry picked from commit ad7c19f) # Conflicts: # x-pack/heartbeat/monitors/browser/source/validatepackage.go # x-pack/heartbeat/monitors/browser/source/validatepackage_test.go * Fix license * Fix notice Co-authored-by: Andrew Cholakian <andrew@andrewvc.com> (cherry picked from commit 5f49358)

…26250) (#27083) * [Heartbeat] Fix broken invocation of synth package (#26228) Fixes invocation of synthetics broken in #26188 Additionally, for dev purposes, this lets accepts a synthetics version of file:/// as valid to help with debugging development versions of the synthetics agent. Never released, so no changelog needed Still just sticking with unit tests here since testing more deeply with synthetics (esp. master where this is a problem) is a larger problem than we're equipped to handle ATM. (cherry picked from commit ad7c19f) # Conflicts: # x-pack/heartbeat/monitors/browser/source/validatepackage.go # x-pack/heartbeat/monitors/browser/source/validatepackage_test.go * Fix license * Fix notice Co-authored-by: Andrew Cholakian <andrew@andrewvc.com> (cherry picked from commit 5f49358) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Fix broken invocation of synth package

63e4ac5

andrewvc added Heartbeat [zube]: In Progress Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Jun 10, 2021

andrewvc requested a review from vigneshshanmugam June 10, 2021 00:47

andrewvc self-assigned this Jun 10, 2021

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 10, 2021

vigneshshanmugam reviewed Jun 10, 2021

View reviewed changes

Add tests, cleanup

68af7d8

andrewvc added the v7.14.0 label Jun 10, 2021

vigneshshanmugam approved these changes Jun 10, 2021

View reviewed changes

andrewvc marked this pull request as ready for review June 10, 2021 22:24

andrewvc merged commit ad7c19f into elastic:master Jun 10, 2021

andrewvc deleted the fix-broken-invoke branch June 10, 2021 22:35

mergify bot mentioned this pull request Jun 10, 2021

[Heartbeat] Fix broken invocation of synth package (backport #26228) #26250

Merged

andrewvc added test-plan Add this PR to be manual test plan [zube]: Done and removed [zube]: In Progress labels Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Fix broken invocation of synth package #26228

[Heartbeat] Fix broken invocation of synth package #26228

andrewvc commented Jun 10, 2021 •

edited

Loading

elasticmachine commented Jun 10, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

vigneshshanmugam Jun 10, 2021

andrewvc Jun 10, 2021

vigneshshanmugam Jun 10, 2021

vigneshshanmugam left a comment

vigneshshanmugam Jun 10, 2021 •

edited

Loading

elasticmachine commented Jun 10, 2021

andrewvc commented Jun 10, 2021

mergify bot commented Jun 10, 2021

[Heartbeat] Fix broken invocation of synth package #26228

[Heartbeat] Fix broken invocation of synth package #26228

Conversation

andrewvc commented Jun 10, 2021 • edited Loading

Checklist

How to test this PR locally

elasticmachine commented Jun 10, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

vigneshshanmugam Jun 10, 2021

Choose a reason for hiding this comment

andrewvc Jun 10, 2021

Choose a reason for hiding this comment

vigneshshanmugam Jun 10, 2021

Choose a reason for hiding this comment

vigneshshanmugam left a comment

Choose a reason for hiding this comment

vigneshshanmugam Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jun 10, 2021

andrewvc commented Jun 10, 2021

mergify bot commented Jun 10, 2021

andrewvc commented Jun 10, 2021 •

edited

Loading

elasticmachine commented Jun 10, 2021 •

edited by jenkins-beats-ci bot

Loading

vigneshshanmugam Jun 10, 2021 •

edited

Loading